[api-extractor] Extract source file locations for relevant API items #3590

zelliott · 2022-08-18T15:35:26Z

Summary

Fixes #895.

This PR adds a new sourceFile property to certain API items that tracks the source file where that item was declared. This allows an API reference site to include links back to the original source code. It largely follows the design agreed upon by @octogonz and @ecraig12345.

Details

`ApiDeclaredItem.sourceFile`

A new sourceFile property was added to the ApiDeclaredItem class as opposed to some other API item class or mixin because it only makes sense for declared items (items with source code excerpts).
We first try to serialize the path to the original .ts source file. If we can't resolve the original .ts source file for some reason, then we fall back to the .d.ts file. If this also fails for some reason (should be impossible), then the property is undefined.
The path is relative to the <projectFolder> token as defined in the api-extractor.json config file.

`SourceMapper` changes

MessageRouter uses SourceMapper today to resolve mappings between .d.ts and .ts files. I made a series of changes to the SourceMapper to make it more generic such that now both MessageRouter and ApiModelGenerator can use it.

Tracking source file position

#895 discussed tracking other information in additional to the source file (e.g. start/end position, line number, column number, buffer offset, etc). This PR only tracks the source file because that's all that was requested in the original issue (#895 (comment)) and that's sufficient for my use case.

Scenarios when `sourceFile` points to a `.d.ts` file

There are only two cases in the api-extractor-scenarios tests where the sourceFile property for an API item points to a .d.ts file:

The API item is in a separate package, but bundledPackages is turned on. We don't have access to the original .ts files, so .d.ts is the best we can do.
There are simply no .ts source files due to the nature of the test. In the excerptTokens test, there are no .ts source files (only .d.ts that were manually written), and thus the sourceFile property for API items point to .d.ts files.

`AstNamespaceImport` items

AstNamespaceImport items are special because they aren't declared as namespaces in the original source files with the namespace keyword. Instead, they are "synthetically" created by API Extractor to represent import * as ns from './file' statements.

They are represented in the .api.json as AstNamespace items, and their sourceFile points to the file where the import * as ns from './file' statement is. I considered the sourceFile pointing to the './file' file instead, but I thought this would be more confusing for users of an API reference site.

API items spanning multiple files

Today, it's not possible for declarations across multiple .ts files to be represented by a single API item. But this may be possible in the future. In this case, which source file do we track?

I don't think it's worth trying to design for this right now, largely because the .api.json as-is (before this feature) is already fundamentally incompatible with API items spanning multiple files (and even multiple declarations). For example, two namespaces with the same name in the same file are represented today as one AstNamespace item. However, the docComment and excerptToken fields are from the first namespace, the second is ignored (even though the AstNamespace still has the members from both namespaces). This feels like an pre-existing limitation in the .api.json that this feature doesn't make more complicated to solve.

If we did want to solve it, we'd probably have to change the API item hierarchy such that a single API item can have multiple "declarations". The ApiDeclaredItem class could be updated to have a list of docComment, excerptTokens, and sourceFile fields, probably bundled together in some new intermediary interface (so that you know which doc comment is associated with which excerpt tokens and source file). But this would be a big breaking change that is probably smart to land separately.

JSON verbosity & stability

This feature increases the size of the .api.json by adding a new field to essentially all API items and decreases the stability of the .api.json (i.e. an API item moved between two local files will cause an .api.json change).

How it was tested

Ran rush rebuild and inspected all of the .api.json files.

apps/api-extractor/src/collector/MessageRouter.ts

libraries/api-extractor-model/src/items/ApiDeclaredItem.ts

build-tests/api-extractor-scenarios/etc/spanSorting/api-extractor-scenarios.api.json

apps/api-extractor/src/collector/SourceMapper.ts

libraries/api-extractor-model/src/items/ApiDeclaredItem.ts

zelliott · 2022-08-30T22:00:21Z

common/reviews/api/api-extractor-model.api.md

@@ -7,6 +7,7 @@
 import { DeclarationReference } from '@microsoft/tsdoc/lib-commonjs/beta/DeclarationReference';
 import { DocDeclarationReference } from '@microsoft/tsdoc';
 import { IJsonFileSaveOptions } from '@rushstack/node-core-library';
+import { JsonObject } from '@rushstack/node-core-library';


I'm not sure why this is appearing... 🤔. Perhaps an API Extractor bug.

libraries/api-extractor-model/src/items/ApiDeclaredItem.ts

zelliott · 2022-09-19T20:56:18Z

libraries/api-extractor-model/src/model/SourceLocation.ts

+      return undefined;
+    }
+
+    return path.join(this._projectFolderUrl, this._fileUrlPath);


Note that path.join doesn't quite work here, it shouldn't be used to construct URLs. For example, it will turn "http://something" into "http:/something".

But simple concatenation doesn't work in all cases either, because this._fileUrlPath can include "../" segments (see the api-extractor-scenarios bundledPackages test scenario).

Would the URL constructor work here?

It should solve the outlined issue, e.g. new URL("../path", "http://something/abc").href => http://something.com/path

I'm not sure if it quite solves the issue above. new URL("path", "http://something.com/abc").href => "http://something.com/path", but we'd want "http://something.com/abc/path".

this. _fileUrlPath is always relative to the <projectFolder>. Thus, it should only include "../" segments if it's pointing to a file outside of <projectFolder>. I'm wondering if maybe this can only happen if it's pointing to a .d.ts file - as in the case of the bundledPackages test scenario. If that's the case, then perhaps SourceLocation.fileUrl can return undefined for .d.ts files, as I don't believe they're typically checked into source control, so a constructed URL wouldn't be valid anyway? Then we can just use simple concatenation here.

@octogonz - what do you think?

I'm not sure if it quite solves the issue above. new URL("path", "http://something.com/abc").href => "http://something.com/path", but we'd want "http://something.com/abc/path".

If you include a trailing slash in the base, it will append the path as you'd want it.
e.g. new URL("path", "http://something.com/abc/").href => "http://something.com/abc/path"

Sorry if I'm wasting your time with this, I'm not very familiar with the internals of this project 😓

Oh nice! It seems like new URL could work well in this case then. And you're not wasting my time at all!

Still not sure if we want to handle paths to .d.ts files separately, but In the meantime I can update this to use new URL instead, as it's strictly better than path.join.

(Keep in mind that new URL("Äpfel", "http://something.com/abc/").href prints http://something.com/abc/%C3%84pfel )

octogonz · 2022-10-07T01:58:21Z

apps/api-extractor/src/schemas/api-extractor-template.json

+    // "includeForgottenExports": false,
+
+    /**
+     * The URL to the `<projectFolder>` token where the project's source code can be viewed on a website like GitHub or


How does this relate to the <projectFolder> token? The serialized paths are relative to the <projectFolder> value, but otherwise the projectFolderUrl is a URL not a disk path.

The doc comment mentions the <projectFolder> token because this is the URL to the projectFolder folder on a source code viewing website like GitHub. Might this be less confusing if I use projectFolder instead of <projectFolder>? Maybe that's where the confusion is coming from?

Updated this comment.

libraries/api-extractor-model/src/items/ApiDeclaredItem.ts

octogonz · 2022-10-07T02:14:36Z

apps/api-extractor/src/generators/ApiModelGenerator.ts

+      pos: declaration.pos
+    });
+
+    return path.relative(this._collector.extractorConfig.projectFolder, sourceLocation.sourceFilePath);


@zelliott Here we are computing a filesystem path, but then returning it as a "URL path". For example, if the source file is called "src/@bad+folder/Example.ts" then at some point it may need to get encoded into something like src/%40bad%2Bfolder/Example.ts to avoid trouble in the web server.

How we encode it (URI? IRI?) perhaps depends on the particular target website. So I think it maybe makes sense to preserve a "raw" URL-like path here, and document clearly in the API model that the consumer is responsible for URL encoding. And we could demonstrate this by performing such encoding appropriately in API Documenter, at the step where it emits its Markdown hyperlinks.

On Windows OS, path.relative() will in fact include backslashes instead of slashes, which needs to be addressed here, though.

So I think it maybe makes sense to preserve a "raw" URL-like path here, and document clearly in the API model that the consumer is responsible for URL encoding.

If I understand you correctly, you're suggesting to just update the SourceLocation.fileUrl doc comment to clarify that URL encoding is a responsibility of the consumer?

I'm wondering if URL construction altogether should be a responsibility of the consumer. The construction we're currently performing feels a little fragile, and I'm not sure if it's possible to improve it. See the discussion at #3590 (comment).

Updated to use path.posix.relative instead.

zelliott requested review from iclanton, octogonz, apostolisms and D4N14L as code owners August 18, 2022 15:35

zelliott commented Aug 18, 2022

View reviewed changes

apps/api-extractor/src/collector/MessageRouter.ts Show resolved Hide resolved

octogonz reviewed Aug 23, 2022

View reviewed changes

libraries/api-extractor-model/src/items/ApiDeclaredItem.ts Outdated Show resolved Hide resolved

octogonz reviewed Aug 23, 2022

View reviewed changes

build-tests/api-extractor-scenarios/etc/spanSorting/api-extractor-scenarios.api.json Outdated Show resolved Hide resolved

octogonz reviewed Aug 23, 2022

View reviewed changes

apps/api-extractor/src/collector/SourceMapper.ts Outdated Show resolved Hide resolved

octogonz reviewed Aug 24, 2022

View reviewed changes

libraries/api-extractor-model/src/items/ApiDeclaredItem.ts Show resolved Hide resolved

zelliott commented Aug 30, 2022

View reviewed changes

zelliott requested a review from octogonz August 30, 2022 22:48

zelliott force-pushed the source-file branch from c5bc4e3 to cb99b3e Compare September 14, 2022 15:09

zelliott commented Sep 16, 2022

View reviewed changes

libraries/api-extractor-model/src/items/ApiDeclaredItem.ts Outdated Show resolved Hide resolved

zelliott commented Sep 19, 2022

View reviewed changes

octogonz reviewed Oct 7, 2022

View reviewed changes

libraries/api-extractor-model/src/items/ApiDeclaredItem.ts Show resolved Hide resolved

octogonz reviewed Oct 7, 2022

View reviewed changes

zelliott added 11 commits October 11, 2022 09:03

Add a new sourceFile field to ApiDeclaredItem

d5e0ea4

Bumped .api.json version and added a test case.

b117031

A bit more cleanup

5790e5f

Responded to PR feedback

dc37492

Ran rush rebuild one more time

2bff1ee

Remove unnecessary question mark operator

1178704

Rebased off of recent changes

920a125

Remove unnecessary code

1361612

Fixes to ApiDeclaredItem

0154e43

Use URL class

03a7354

Rebased off master and responded to PR comments

3dac1bb

zelliott force-pushed the source-file branch from 0c5ac31 to 3dac1bb Compare October 11, 2022 16:29

octogonz approved these changes Oct 11, 2022

View reviewed changes

octogonz merged commit 2f8d789 into microsoft:main Oct 11, 2022

donmccurdy mentioned this pull request Oct 18, 2022

Remove TypeDoc dependency donmccurdy/glTF-Transform#342

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api-extractor] Extract source file locations for relevant API items #3590

[api-extractor] Extract source file locations for relevant API items #3590

zelliott commented Aug 18, 2022 •

edited

Loading

zelliott Aug 30, 2022 •

edited

Loading

zelliott Sep 19, 2022

almeidx Sep 20, 2022

zelliott Sep 20, 2022

almeidx Sep 20, 2022

zelliott Sep 20, 2022

octogonz Oct 11, 2022

octogonz Oct 7, 2022

zelliott Oct 10, 2022

zelliott Oct 11, 2022

octogonz Oct 7, 2022

octogonz Oct 7, 2022

zelliott Oct 10, 2022 •

edited

Loading

zelliott Oct 11, 2022

[api-extractor] Extract source file locations for relevant API items #3590

[api-extractor] Extract source file locations for relevant API items #3590

Conversation

zelliott commented Aug 18, 2022 • edited Loading

Summary

Details

ApiDeclaredItem.sourceFile

SourceMapper changes

Tracking source file position

Scenarios when sourceFile points to a .d.ts file

AstNamespaceImport items

API items spanning multiple files

JSON verbosity & stability

How it was tested

zelliott Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelliott Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelliott commented Aug 18, 2022 •

edited

Loading

`ApiDeclaredItem.sourceFile`

`SourceMapper` changes

Scenarios when `sourceFile` points to a `.d.ts` file

`AstNamespaceImport` items

zelliott Aug 30, 2022 •

edited

Loading

zelliott Oct 10, 2022 •

edited

Loading